Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds @kbn/utils package #76518

Merged
merged 8 commits into from
Sep 15, 2020
Merged

Adds @kbn/utils package #76518

merged 8 commits into from
Sep 15, 2020

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented Sep 2, 2020

This moves some common utility functions to obtain the repository root, paths (config/data), and Kibana package.json to a @kbn/utils package. Moving these existing functions allows them to be used in production across plugins and packages.

Related to #76874
Blocks #75521

@tylersmalley tylersmalley force-pushed the kbn-utils branch 4 times, most recently from 8e731a9 to 1d1b0e8 Compare September 8, 2020 00:34
@tylersmalley tylersmalley force-pushed the kbn-utils branch 3 times, most recently from 2874cb6 to cbdacce Compare September 9, 2020 00:41
Moves common utility functions to obtain the repository root,
paths (config/data), and Kibana package.json to a @kbn/utils package.
Moving these existing functions allows them to be used in production, in
other packages because Kibana.

Signed-off-by: Tyler Smalley <[email protected]>
@tylersmalley tylersmalley added Team:Operations Team label for Operations Team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0 labels Sep 9, 2020
@tylersmalley tylersmalley marked this pull request as ready for review September 9, 2020 15:17
@tylersmalley tylersmalley requested review from a team as code owners September 9, 2020 15:17
@tylersmalley tylersmalley requested a review from a team September 9, 2020 15:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@tylersmalley tylersmalley added the release_note:skip Skip the PR/issue when compiling release notes label Sep 9, 2020
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security team changes LGTM

x-pack/test/spaces_api_integration/common/config.ts

src/core/server/server.ts Outdated Show resolved Hide resolved
packages/kbn-utils/package.json Outdated Show resolved Hide resolved
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Telemetry changes LGTM!

Signed-off-by: Tyler Smalley <[email protected]>
packages/kbn-utils/tsconfig.json Outdated Show resolved Hide resolved
packages/kbn-utils/tsconfig.json Outdated Show resolved Hide resolved
packages/kbn-utils/README.md Show resolved Hide resolved
packages/kbn-utils/package.json Outdated Show resolved Hide resolved
packages/kbn-utils/README.md Show resolved Hide resolved
@@ -21,7 +21,7 @@ import { existsSync } from 'fs';
import { join } from 'path';

import { Logger } from '../cli_plugin/lib/logger';
import { getConfigDirectory, getDataPath } from '../core/server/path';
import { getConfigDirectory, getDataPath } from '@kbn/utils';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay that getDataPath ignores a value configured in kibana.yml? We've even got an issue for this #43539

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to first move this over, then resolve that issue - might wait for #76874 to assist with that.

@@ -0,0 +1,3 @@
# @kbn/utils

Shared server-side utilities shared across packages and plugins.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw are we sure that we'd never need Kibana-specific browser utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible but thought it would be best to keep it focused to start.

Copy link
Contributor

@spalger spalger Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect we will quickly start hearing stories of @kbn/utils gets pulled into the browser builds accidentally and failing because things like REPO_ROOT don't work in the browser (and will throw an error on page load, maybe even at build time).

It seems like we'll need to have separate entry points, like @kbn/utils/node and @kbn/utils/browser, which have slightly different APIs but share the exports they can. Still, something we can do in a follow-up as needed.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for stack monitoring, as long as CI is passing!

@tylersmalley tylersmalley requested a review from a team as a code owner September 10, 2020 16:40
Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Canvas changes lgtm

@@ -128,6 +128,7 @@
/packages/kbn-test/ @elastic/kibana-operations
/packages/kbn-ui-shared-deps/ @elastic/kibana-operations
/packages/kbn-es-archiver/ @elastic/kibana-operations
/packages/kbn-utils/ @elastic/kibana-operations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to remove this before back-porting.

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok for the platform changes

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I don't love jest-when and wonder if it might actually be more reasonable to start with a simple helper defined in x-pack/plugins/canvas/server/routes/shareables/download.test.ts rather than pulling in a package which bundles bunyan.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
enterpriseSearch 430.1KB +49.0B 430.0KB

page load bundle size

id value diff baseline
upgradeAssistant 64.8KB +49.0B 64.7KB

distributable file count

id value diff baseline
default 45574 +5 45569
oss 27252 +6 27246
total +11

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tylersmalley tylersmalley merged commit 9acf8d2 into elastic:master Sep 15, 2020
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Sep 15, 2020
Moves common utility functions to obtain the repository root,
paths (config/data), and Kibana package.json to a @kbn/utils package.
Moving these existing functions allows them to be used in production, in
other packages because of Kibana.

Signed-off-by: Tyler Smalley <[email protected]>
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Sep 15, 2020
Moves common utility functions to obtain the repository root,
paths (config/data), and Kibana package.json to a @kbn/utils package.
Moving these existing functions allows them to be used in production, in
other packages because of Kibana.

Signed-off-by: Tyler Smalley <[email protected]>
tylersmalley pushed a commit that referenced this pull request Sep 15, 2020
Moves common utility functions to obtain the repository root,
paths (config/data), and Kibana package.json to a @kbn/utils package.
Moving these existing functions allows them to be used in production, in
other packages because of Kibana.

Signed-off-by: Tyler Smalley <[email protected]>

* [@kbn/utils] Adds missing dependency (#77536)

Signed-off-by: Tyler Smalley <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 16, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (95 commits)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058)
  [@kbn/utils] Adds missing dependency (elastic#77536)
  Add the Enterprise Search logo to the Overview search result (elastic#77514)
  [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482)
  [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402)
  Adds @kbn/utils package (elastic#76518)
  Map layout changes (elastic#77132)
  [Security Solution] [Detections] EQL Rule Creation (elastic#76831)
  Adding test user to maps tests under documents source folder  (elastic#77245)
  Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.